padding between bool bitfields? Why use bitfields in any case?

The padding was for brute-force stress-testing only while working on #5213. I just reverted it here (c02d2cd). It's easy enough to redo if/as needed.

(The bitfields are just to keep the memory footprint small.)

std::move

Done: 99b6572

Thanks for catching this!

You could do this here:

template <typename T, typename D>
struct property_cpp_function__version6_pointer_something {
    template <typename PM, detail::must_be_member_function_pointer<PM> = 0>
    static cpp_function readonly(PM pm, const handle &hdl) {
        detail::type_info *tinfo = detail::get_type_info(typeid(T), /*throw_if_missing=*/true);
        if (tinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
            return cpp_function(
                [pm](handle c_hdl) -> std::shared_ptr<drp> {
                    std::shared_ptr<T> c_sp = detail::type_caster<
                        std::shared_ptr<T>>::shared_ptr_with_responsible_parent(c_hdl);
                    D ptr = (*c_sp).*pm;
                    return std::shared_ptr<drp>(c_sp, ptr);
                },
                is_method(hdl));
        }
        return property_cpp_function<T, D, void>::readonly(std::move(pm), hdl);
    }

    template <typename PM, detail::must_be_member_function_pointer<PM> = 0>
    static cpp_function read(PM pm, const handle &hdl) {
        return readonly(pm, hdl);
    }

    template <typename PM, detail::must_be_member_function_pointer<PM> = 0>
    static cpp_function write(PM pm, const handle &hdl) {
        detail::type_info *tinfo = detail::get_type_info(typeid(T), /*throw_if_missing=*/true);
        if (tinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
            return cpp_function([pm](T &c, D value) { c.*pm = std::move<D>(value); },
                                is_method(hdl));
        }
        return property_cpp_function<T, D, void>::write(std::move(pm), hdl);
    }
};

template <typename T, typename D>
struct property_cpp_function<T,
                             D,
                             detail::enable_if_t<detail::all_of<
                                 detail::none_of<std::is_pointer<D>,
                                                 std::is_array<D>,
                                                 detail::is_instantiation<std::unique_ptr, D>,
                                                 detail::is_instantiation<std::shared_ptr, D>>,
                                 detail::both_t_and_d_use_type_caster_base<T, D>>::value>>
: public property_cpp_function__version6_pointer_something {};

Done: 16cf7ad

That is surely easier to read and nicely enabled this follow-on commit: 0bcfbd4

Do these need to be ifdef guarded?

It's a choice (for the most part at least).

I did it that way so that it is as obvious as possible that pybind11 behaves almost exactly as before when PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT is not defined.

(The few tiny exceptions are a compromise, #ifdef clutter vs. tiny pieces from the smart_holder code leaking in.)

I prefer to keep this as-is, unless you believe it's better to not have conditional #includes.

Why use a switch here? To catch policy enum changes, or what? Why not use a switch above?

To catch policy enum changes

Yes, that exactly was the idea (similar to this existing code on master):

Ideally we'd have more like comprehensive switch like that, to guard against accidents and misunderstandings. (I made a partial pass in the past, but the gain isn't that big, and there were always bigger fish to fry...)

Positive tests are easier to follow. This function is only valid for return_value_policy::copy

Done: cb6a85f

The list of conditions started small and grew over time (to keep existing client code happy). TBH I didn't fully realize that only copy is invalid now.

nit. naming. loaded_as vs. load_as?

It seems like most uses of reinterpret_cast<void*>(&...) in this cl should be changed to static_cast<void*>(&...) instead.

Oh, sorry, these came in through PR #5272 yesterday, which was merged already on master. — I'm not using rebase and push --force here (or anywhere really) because that invalidates a lot of commit hashes, and then I have no way to easily refer to commits from comments.

The best way to review here is to start with the "Files changed" tab near the top of this PR.

While working on PR #5272, I tried static_cast<void *> and it built successfully, but clang-tidy (clang version 18) still complained. reinterpret_cast was to only way to make it happy. — I didn't try to understand exactly why. — I was thinking the reinterpret_casts are better than // NOLINTs.

The best way to review here is to start with the "Files changed" tab near the top of this PR.

Oh, sorry, I just now realized it doesn't look the way I was expecting. Ugh!

Here is a helper PR showing the net diff (what I was wrongly hoping "Files changed" would show):

https://github.com/pybind/pybind11/pull/5277/files

Ok. So perhaps make this change at head. :)